Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughA single-file update to the Discord bot CRM cog that introduces a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes Discord role-apply gating in the CRM resume flow so the “Apply Discord Roles” button is enabled/usable whenever a Discord target user is known, even if an explicit can_apply_discord_roles flag isn’t provided.
Changes:
- Adds
can_apply_discord_rolesinference toResumeUpdateConfirmationViewand uses it to disable/guard the Apply button. - Extends the “Suggested Discord Roles” embed to show a “Link required” warning when role application isn’t allowed.
- Threads
can_apply_discord_rolesthrough the resume extract/preview flow.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not view.can_apply_discord_roles: | ||
| await interaction.response.send_message( | ||
| "❌ Discord roles can only be applied after linking this contact to a Discord user.", | ||
| ephemeral=True, | ||
| ) | ||
| return |
There was a problem hiding this comment.
The new early return when view.can_apply_discord_roles is false bypasses _audit_apply_roles_event, so denied apply attempts in this state will no longer be audit-logged (previously the missing-linked-user path was audited). Consider moving this check below the _audit_apply_roles_event helper (and recording a "denied" stage like missing_linked_user) or explicitly calling the audit helper before returning.
| self.add_item( | ||
| ResumeApplyDiscordRolesButton(disabled=not self.can_apply_discord_roles) | ||
| ) |
There was a problem hiding this comment.
This change disables the Apply button based on self.can_apply_discord_roles, which will flip existing unit-test expectations (e.g., tests that currently assert the Apply button is enabled even when no Discord link/target user ID is set). Please update/add unit tests to cover both states: disabled when there is no linked/target Discord user, and enabled when discord_role_target_user_id (or a valid link_discord.user_id) is present.
| if not can_apply_discord_roles: | ||
| embed.add_field( | ||
| name="🔒 Link required", | ||
| value=( | ||
| "This contact is not currently linked to a Discord user, so suggested roles " | ||
| "cannot be applied automatically. Link them first in CRM or use " | ||
| "`/link-discord-user`." | ||
| ), | ||
| inline=False, | ||
| ) |
There was a problem hiding this comment.
_build_role_suggestions_embed now conditionally adds the "🔒 Link required" field. There are existing unit tests covering this embed builder, but none appear to assert this new field behavior. Add/adjust tests to verify the field is present when can_apply_discord_roles is false and absent when it’s true, so the UX/validation contract doesn’t regress.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/discord_bot/src/five08/discord_bot/cogs/crm.py (1)
1616-1631:⚠️ Potential issue | 🟠 MajorPreserve explicit
Falseinstead of inferring over it.
bool(can_apply_discord_roles or discord_role_target_user_id or ...)makescan_apply_discord_roles=Falseimpossible once a target user or link payload exists. That changes the constructor contract and can re-enable Apply in flows that intentionally passedFalse. UseNoneas the “infer it” default instead.Suggested fix
- can_apply_discord_roles: bool = False, + can_apply_discord_roles: bool | None = None, @@ - self.can_apply_discord_roles = bool( - can_apply_discord_roles - or discord_role_target_user_id - or (isinstance(link_discord, dict) and bool(link_discord.get("user_id"))) - ) + inferred_can_apply_discord_roles = bool( + discord_role_target_user_id + or (isinstance(link_discord, dict) and bool(link_discord.get("user_id"))) + ) + self.can_apply_discord_roles = ( + inferred_can_apply_discord_roles + if can_apply_discord_roles is None + else can_apply_discord_roles + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py` around lines 1616 - 1631, The constructor currently forces can_apply_discord_roles to True whenever discord_role_target_user_id or a link_discord payload exists by assigning self.can_apply_discord_roles = bool(can_apply_discord_roles or discord_role_target_user_id or ...); change the parameter type to Optional[bool] (allow None) and preserve explicit False by using the passed value when not None—i.e., if can_apply_discord_roles is not None assign it directly, otherwise infer from discord_role_target_user_id or link_discord (convert that inference to bool). Update the constructor signature and the assignment for self.can_apply_discord_roles accordingly (refer to the __init__ parameter can_apply_discord_roles, the assignment line, discord_role_target_user_id, and link_discord).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py`:
- Around line 1654-1656: The Apply button is being re-enabled in
ResumeEditDiscordRolesModal.on_submit regardless of the original gate; update
that handler to use the same predicate as the initial add_item call: when
resetting the ResumeApplyDiscordRolesButton, compute disabled using not
(self.can_apply_discord_roles and bool(normalized)) (or equivalent) instead of
not bool(normalized) so the button stays disabled when
self.can_apply_discord_roles is false; adjust the code in
ResumeEditDiscordRolesModal.on_submit where ResumeApplyDiscordRolesButton is
recreated/updated to use this combined predicate.
- Around line 1288-1293: The denied branch for view.can_apply_discord_roles
returns before emitting an audit record; call the existing auditing helper
before returning so the denial is recorded. Specifically, invoke the same audit
routine used elsewhere (e.g., call/await _audit_apply_roles_event(...) or the
project audit helper used by other paths) with a denial status/reason and the
same context (interaction, view/contact, actor) immediately before the
interaction.response.send_message and return, ensuring the audit call is awaited
and uses the worker audit ingest helper used elsewhere in this module.
---
Outside diff comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm.py`:
- Around line 1616-1631: The constructor currently forces
can_apply_discord_roles to True whenever discord_role_target_user_id or a
link_discord payload exists by assigning self.can_apply_discord_roles =
bool(can_apply_discord_roles or discord_role_target_user_id or ...); change the
parameter type to Optional[bool] (allow None) and preserve explicit False by
using the passed value when not None—i.e., if can_apply_discord_roles is not
None assign it directly, otherwise infer from discord_role_target_user_id or
link_discord (convert that inference to bool). Update the constructor signature
and the assignment for self.can_apply_discord_roles accordingly (refer to the
__init__ parameter can_apply_discord_roles, the assignment line,
discord_role_target_user_id, and link_discord).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ed05279-fcbc-468e-ad8e-73f7d4da3f55
📒 Files selected for processing (1)
apps/discord_bot/src/five08/discord_bot/cogs/crm.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.can_apply_discord_roles = bool( | ||
| can_apply_discord_roles | ||
| or discord_role_target_user_id | ||
| or (isinstance(link_discord, dict) and bool(link_discord.get("user_id"))) | ||
| ) |
There was a problem hiding this comment.
can_apply_discord_roles is computed with an or chain, so an explicitly provided False is overridden when discord_role_target_user_id or link_discord['user_id'] is present. If the intent is to preserve an explicit flag (True/False) and only infer when it’s not provided, consider making the parameter bool | None = None and computing self.can_apply_discord_roles = inferred if can_apply_discord_roles is None else can_apply_discord_roles.
| if not view.can_apply_discord_roles: | ||
| _audit_apply_roles_event( | ||
| "denied", | ||
| "missing_linked_user", | ||
| {"reason": "button_disabled_for_role_application"}, |
There was a problem hiding this comment.
This new early-return branch for unlinked contacts is important behavior (and was added specifically to guard a regression), but there’s no unit test covering the callback being triggered while view.can_apply_discord_roles is false (e.g., via a stale interaction/custom_id). Adding a test that asserts the ephemeral error message (and, if possible, that audit is called) would help prevent this from regressing again.
Description
This change fixes CRM resume role-apply behavior so the "Apply Discord Roles" button can be used whenever a Discord target user is known, even when
can_apply_discord_rolesis not explicitly passed.It infers role-apply capability from
discord_role_target_user_idand validlink_discordpayloads, and preserves the existing explicit flag path.It also keeps the UI and validation consistent by disabling the apply button when role application is not allowed and surfacing the existing warning flow.
Related Issue
#199
How Has This Been Tested?
Not run in this environment after this change; the upstream CI failure pinpointed this exact callback path, and the branch includes the patch to address that regression path.
Summary by CodeRabbit
Release Notes